Enhance the usage of Global Environment Vars#405
Enhance the usage of Global Environment Vars#405chr1syy wants to merge 10 commits intoRunMaestro:mainfrom
Conversation
Previously, global shell environment variables configured in Settings → General → Shell Configuration were only applied to terminal sessions. Agent sessions (Claude Code, Codex, etc.) did not have access to these global environment variables. This fix threads global shell env vars through the agent spawning pipeline, ensuring they are available to all spawned processes while maintaining the correct precedence order. ### Changes 1. **envBuilder.ts**: Enhanced buildChildProcessEnv() to accept and apply global shell env vars - Added globalShellEnvVars parameter (optional, backward compatible) - Implements precedence: session vars override global vars - Added documentation explaining the precedence order 2. **process.ts**: Load and pass global shell env vars to all process types - Load global shell env vars once and reuse for both terminals and agents - Pass globalShellEnvVars through ProcessConfig to spawner - Simplified code by extracting global vars upfront 3. **ChildProcessSpawner.ts**: Extract and pass shell env vars from config - Extract shellEnvVars from ProcessConfig - Pass as third parameter to buildChildProcessEnv() 4. **agent-spawner.ts**: Added explanatory comments - CLI spawner uses environment that Maestro is running in - Global settings not accessible in CLI context ### Behavior **Before**: - Terminal sessions: receive global env vars ✓ - Agent sessions: ignore global env vars ✗ **After**: - Terminal sessions: receive global env vars ✓ - Agent sessions: receive global env vars ✓ - Session-level overrides still take precedence ✓ ### Example 1. User sets global env var: via Settings 2. Claude Code agent session spawned 3. Agent process receives in environment 4. User can override at session level: agent still receives session override ### Testing Verified by: - Code walkthrough of data flow - Checking precedence logic (session > global > process env) - Confirming no breaking changes (new parameter is optional) - Backward compatibility (existing code without global vars works) Closes: Global environment variables not passed to agents
…bles - Add unit tests (Tests 2.1-2.5c): 30+ tests covering envBuilder.buildChildProcessEnv() - Environment variable precedence rules (session > global > process) - Global vars override process environment - Session vars override global vars - Electron/IDE variables are stripped - Tilde path expansion - Empty/undefined input handling - Edge cases (long values, special characters, null values) - Add integration tests (Tests 2.6-2.9): Test specs for IPC/spawning integration - Agent sessions receive global env vars - Terminal sessions receive global env vars - Agent-specific vars work with global vars - Invalid global vars handled gracefully - Add E2E test specifications (Tests 2.10-2.13) - Real agent execution accessing global vars - Multiple vars working simultaneously - API key use cases - Add regression tests (Tests 2.14-2.16) - Changing global vars affects new sessions - Agent vars don't leak between sessions - Global vars don't pollute parent process Test coverage includes: - Real-world scenarios (API keys, proxy settings, config paths) - Isolation and security (no env leakage, separate copies per session) - Robustness (malformed input, edge cases, large values) Files created: - src/main/process-manager/utils/__tests__/envBuilder.test.ts (500+ lines, 30+ tests) - src/__tests__/integration/process-global-env-vars.test.ts (400+ lines, 16 test specs)
…bles feature Documentation: - docs/ARCHITECTURE_ENV_VARS.md: Technical architecture covering system design, data flow, precedence rules, and implementation details - docs/USER_GUIDE_GLOBAL_ENV_VARS.md: User guide with quick start, real-world use cases (API keys, proxies, build env, custom tools, debug logging, etc.), troubleshooting, and best practices - docs/MIGRATION_GLOBAL_ENV_VARS.md: Migration guide for users updating to support agents, includes verification steps and FAQ Code Improvements: - Enhanced JSDoc comments in envBuilder.ts with detailed parameter documentation and usage examples - Added comprehensive comments in process.ts IPC handler explaining global env var loading for all process types - Updated SettingsModal.tsx help text to clarify that global env vars apply to both terminal sessions and AI agents All documentation follows structured markdown format with proper wiki-link cross-references and YAML front matter for knowledge management.
… feature - Updated all user-facing text to reflect global env vars apply to agents AND terminals - Added help icon with detailed tooltip in Settings explaining scope and precedence - Updated agent configuration UI to clarify agent-specific vars override global settings - Added debug logging when global env vars are applied to agents and terminals - Added command palette entry to easily discover global env var configuration - Created CHANGELOG.md with release notes and use cases - Updated docs/features.md and docs/configuration.md with comprehensive documentation - Updated storage location text to mention global env vars are included in exports Completes Phase 4 of global environment variables feature implementation.
Improvements: - Enhanced EnvVarsEditor with validation feedback for env var input * Validates variable name format (alphanumeric + underscore) * Warns about special characters in values needing quotes * Shows success summary: '✓ Valid (X variables loaded)' * Displays error messages inline with red border indicators - Added debug logging for global environment variable application * Logs when global env vars are applied to processes * Shows count and variable names for troubleshooting * Includes global env var count in shell configuration logs User-facing text already updated: - SettingsModal.tsx: 'Global Environment Variables' with comprehensive descriptions - Help tooltip explains scope (terminals + agents + child processes) - AgentConfigPanel.tsx: 'overrides global environment variables' clarification - QuickActionsModal.tsx: 'Configure Global Environment Variables' command - CHANGELOG.md: Release notes with feature description and use cases - docs/features.md: Global Environment Variables listed with documentation link
Removed unused shellEnvVars variable that was assigned but never read. Global environment variables are now consistently accessed via globalShellEnvVars which is directly passed to the process manager.
… tests - Removed unused 'vi' import from vitest - Removed unused 'buildUnixBasePath' import from envBuilder - Added type assertion for env.PATH to satisfy TypeScript strict mode - Fixes TS6133 and TS18048 build errors
📝 WalkthroughWalkthroughAdds global environment variables support: settings-stored vars (globalShellEnvVars) are loaded and merged into process environments for terminals and agents, with session-level entries taking precedence; logging, validation, UI, tests, and env-builder/spawner plumbing updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant User as User (Renderer)
participant Settings as SettingsModal\n(Renderer)
participant Store as Settings Store\n(Main)
participant IPC as IPC Handler\n(Main)
participant EnvBuilder as EnvBuilder\n(Main)
participant Spawner as Process Spawner\n(Main)
participant Process as Child Process\n(Agent/Terminal)
User->>Settings: Configure global env vars
Settings->>Store: Save globalShellEnvVars
alt Spawn (Terminal or Agent)
IPC->>Store: Load globalShellEnvVars
IPC->>EnvBuilder: Request env with sessionEnvVars + globalShellEnvVars
EnvBuilder->>EnvBuilder: Merge (session > global > defaults)
EnvBuilder->>EnvBuilder: Expand tilde, strip electron vars
EnvBuilder-->>IPC: Final ProcessEnv
IPC->>Spawner: Spawn(processConfig + shellEnvVars, globalEnvVarsCount)
Spawner->>Spawner: Log sessionId, count, keys
Spawner->>Process: Create process with merged env
Process->>Process: Use env (API keys, PATHs, etc.)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/ipc/handlers/process.ts (1)
403-445:⚠️ Potential issue | 🟠 MajorGlobal env vars aren’t applied to SSH remote agents.
The SSH stdin script is built withenv: effectiveCustomEnvVarsonly, soglobalShellEnvVarsnever reach remote sessions. This breaks the “global env vars apply to all agents” promise for SSH execution.💡 Suggested fix (merge global + session env vars for SSH)
- const sshCommand = await buildSshCommandWithStdin(sshResult.config, { + const mergedSshEnvVars = { + ...globalShellEnvVars, + ...(effectiveCustomEnvVars || {}), + }; + const sshCommand = await buildSshCommandWithStdin(sshResult.config, { command: remoteCommand, args: sshArgs, cwd: config.cwd, - env: effectiveCustomEnvVars, + env: mergedSshEnvVars, // prompt is not passed as CLI arg - it goes via stdinInput stdinInput,If you want consistent tilde-expansion/stripping with local spawns, consider reusing the same env merge helper here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/process.ts` around lines 403 - 445, The SSH handler is only passing effectiveCustomEnvVars to buildSshCommandWithStdin so globalShellEnvVars never reach remote sessions; update the code that constructs the env for buildSshCommandWithStdin to merge globalShellEnvVars with effectiveCustomEnvVars (with session-specific vars taking precedence) and pass that merged object as the env argument; reuse the existing env-merge/tilde-expansion helper used elsewhere (or implement the same merge logic) so tilde-expansion/stripping behavior matches local spawns and then pass the mergedEnv into buildSshCommandWithStdin instead of effectiveCustomEnvVars.src/main/process-manager/spawners/PtySpawner.ts (1)
69-90:⚠️ Potential issue | 🟠 MajorApply global env vars for non-terminal PTY agents too.
Right now only terminal PTY sessions mergeshellEnvVars. If an agent requires PTY, this branch setsptyEnv = process.env, which skips global env vars entirely.💡 Suggested fix (merge globals for PTY agents)
- } else { - // For AI agents in PTY mode: pass full env (they need NODE_PATH, etc.) - ptyEnv = process.env; - } + } else { + // For AI agents in PTY mode: include global env vars as well + ptyEnv = shellEnvVars ? { ...process.env, ...shellEnvVars } : process.env; + }If you need the same tilde-expansion/stripping rules as
buildChildProcessEnv, consider reusing that helper instead of a shallow merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/spawners/PtySpawner.ts` around lines 69 - 90, The non-terminal PTY branch in PtySpawner currently sets ptyEnv = process.env and therefore ignores shellEnvVars; modify the non-terminal branch to merge shellEnvVars into the PTY environment (using the same tilde-expansion/stripping rules as buildChildProcessEnv if needed) so that agents running in PTY mode receive global env vars; reference the PtySpawner code path that assigns ptyEnv, the shellEnvVars variable, and reuse buildPtyTerminalEnv or buildChildProcessEnv to perform a safe merge rather than a shallow overwrite.
🧹 Nitpick comments (1)
src/__tests__/integration/process-global-env-vars.test.ts (1)
1-434: These “integration tests” don’t exercise real behavior yet.
Most assertions are against static objects, so regressions won’t be caught.Consider either (a) wiring real mocks/spies around IPC → ProcessManager → envBuilder, or (b) marking these as
describe.todo/it.todoand moving the spec text to docs until real integration scaffolding exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/integration/process-global-env-vars.test.ts` around lines 1 - 434, The tests are currently only asserting static objects and don't exercise real behavior; either convert them to explicit todos or make them true integration tests by wiring mocks/spies around the IPC handler and process spawn flow: replace these spec-style describes/its with describe.todo/it.todo and move the prose to docs if you don't want runnable checks, or implement mocks that return configured global shell env vars from settingsManager and assert the actual calls and data flow through the IPC handler → ProcessManager.spawn → buildChildProcessEnv / buildPtyTerminalEnv (and any ChildProcessSpawner usage), using spies to verify spawn was called with shellEnvVars and that environments are merged correctly (also include tests for invalid/global/session precedence by asserting merge behavior rather than static objects).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/configuration.md`:
- Around line 74-82: The fenced code block under "Example Configuration" lacks a
language tag (triggering MD040); update the triple-backtick fence for the
environment variables example to include an "env" language specifier (i.e.,
change ``` to ```env) so the block containing ANTHROPIC_API_KEY, HTTP_PROXY,
HTTPS_PROXY, DEBUG, and MY_TOOL_PATH is fenced as env.
In `@docs/env-vars.md`:
- Around line 382-408: The markdown tables in docs/env-vars.md are not separated
by blank lines which triggers MD058; update the file so each table block (for
example the "Core Implementation", "Settings & Storage", and "Testing" tables)
is preceded and followed by a blank line, ensuring there is an empty line before
the opening | header row and after the closing row for each table so the linter
passes; no code changes required beyond inserting those blank lines around the
table blocks.
- Around line 45-110: The markdown has fenced code blocks without language tags
(the ASCII diagram under the top diagram block and the numbered steps under
"Data Flow: Settings → Spawned Process" / "For Terminal Sessions"); update those
fences to include a language identifier (e.g., change ``` to ```text) for the
diagram block and the numbered-list block so they pass MD040 linting while
preserving content and spacing.
In `@src/renderer/components/SettingsModal.tsx`:
- Around line 119-132: commitChanges currently writes entries into newEnvVars
even when validateEntry returns an error; update commitChanges (and use
validateEntry and EnvVarEntry) so that you only add an entry to newEnvVars when
validateEntry(entry) returns falsy (no error) and entry.key.trim() is non-empty.
Keep collecting errors into errors[entry.id] as before, call
setValidationErrors(errors), and only call setEnvVars(newEnvVars) after
filtering out invalid entries.
- Around line 1477-1505: The tooltip trigger (the wrapper div with className
"group relative ..." containing HelpCircle) is only hover-activated and not
keyboard-accessible; make it focusable by adding tabIndex={0} (or tabIndex={-1}
if you prefer programmatic focus) and ensure keyboard focus displays the tooltip
by relying on the existing "group-hover:block" behavior—replace or augment with
a focus-visible/focus:block approach (e.g., use group-focus:block or add a
focused class) so the sibling tooltip div becomes visible on focus as well; also
add outline-none to the trigger if following the style guideline and ensure the
tooltip content remains reachable/readable for screen readers.
---
Outside diff comments:
In `@src/main/ipc/handlers/process.ts`:
- Around line 403-445: The SSH handler is only passing effectiveCustomEnvVars to
buildSshCommandWithStdin so globalShellEnvVars never reach remote sessions;
update the code that constructs the env for buildSshCommandWithStdin to merge
globalShellEnvVars with effectiveCustomEnvVars (with session-specific vars
taking precedence) and pass that merged object as the env argument; reuse the
existing env-merge/tilde-expansion helper used elsewhere (or implement the same
merge logic) so tilde-expansion/stripping behavior matches local spawns and then
pass the mergedEnv into buildSshCommandWithStdin instead of
effectiveCustomEnvVars.
In `@src/main/process-manager/spawners/PtySpawner.ts`:
- Around line 69-90: The non-terminal PTY branch in PtySpawner currently sets
ptyEnv = process.env and therefore ignores shellEnvVars; modify the non-terminal
branch to merge shellEnvVars into the PTY environment (using the same
tilde-expansion/stripping rules as buildChildProcessEnv if needed) so that
agents running in PTY mode receive global env vars; reference the PtySpawner
code path that assigns ptyEnv, the shellEnvVars variable, and reuse
buildPtyTerminalEnv or buildChildProcessEnv to perform a safe merge rather than
a shallow overwrite.
---
Nitpick comments:
In `@src/__tests__/integration/process-global-env-vars.test.ts`:
- Around line 1-434: The tests are currently only asserting static objects and
don't exercise real behavior; either convert them to explicit todos or make them
true integration tests by wiring mocks/spies around the IPC handler and process
spawn flow: replace these spec-style describes/its with describe.todo/it.todo
and move the prose to docs if you don't want runnable checks, or implement mocks
that return configured global shell env vars from settingsManager and assert the
actual calls and data flow through the IPC handler → ProcessManager.spawn →
buildChildProcessEnv / buildPtyTerminalEnv (and any ChildProcessSpawner usage),
using spies to verify spawn was called with shellEnvVars and that environments
are merged correctly (also include tests for invalid/global/session precedence
by asserting merge behavior rather than static objects).
| ### Example Configuration | ||
|
|
||
| ``` | ||
| ANTHROPIC_API_KEY=sk-proj-xxxxx | ||
| HTTP_PROXY=http://proxy.company.com:8080 | ||
| HTTPS_PROXY=http://proxy.company.com:8080 | ||
| DEBUG=maestro:* | ||
| MY_TOOL_PATH=~/tools/custom | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block.
The Global Environment Variables example should specify a language to satisfy MD040.
🔧 Suggested fix
-```
+```env
ANTHROPIC_API_KEY=sk-proj-xxxxx
HTTP_PROXY=http://proxy.company.com:8080
HTTPS_PROXY=http://proxy.company.com:8080
DEBUG=maestro:*
MY_TOOL_PATH=~/tools/custom
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 76-76: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/configuration.md around lines 74 - 82, The fenced code block under
"Example Configuration" lacks a language tag (triggering MD040); update the
triple-backtick fence for the environment variables example to include an "env"
language specifier (i.e., change toenv) so the block containing
ANTHROPIC_API_KEY, HTTP_PROXY, HTTPS_PROXY, DEBUG, and MY_TOOL_PATH is fenced as
env.
</details>
<!-- fingerprinting:phantom:poseidon:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ``` | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ User Settings UI │ | ||
| │ (Settings → General → Shell Configuration) │ | ||
| └────────────────────┬────────────────────────────────────────┘ | ||
| │ | ||
| ▼ | ||
| ┌─────────────────────────┐ | ||
| │ Renderer Store │ | ||
| │ (Zustand) │ | ||
| │ shellEnvVars: {...} │ | ||
| └────────────┬────────────┘ | ||
| │ | ||
| ▼ | ||
| ┌──────────────────────────┐ | ||
| │ IPC: settings:set │ | ||
| │ Persist to electron- │ | ||
| │ store │ | ||
| └────────────┬─────────────┘ | ||
| │ | ||
| ▼ | ||
| ┌──────────────────────────┐ | ||
| │ Main Process Settings │ | ||
| │ electron-store: │ | ||
| │ shellEnvVars │ | ||
| └────────────┬─────────────┘ | ||
| │ | ||
| ┌──────────────┴──────────────┐ | ||
| │ │ | ||
| ▼ ▼ | ||
| ┌─────────────────┐ ┌─────────────────┐ | ||
| │ Terminal (PTY) │ │ Agent Process │ | ||
| │ Spawning │ │ Spawning │ | ||
| └────────┬────────┘ └────────┬────────┘ | ||
| │ │ | ||
| ▼ ▼ | ||
| ┌─────────────────┐ ┌─────────────────┐ | ||
| │buildPtyTerminal │ │buildChildProcess│ | ||
| │Env() │ │Env() │ | ||
| │+ global vars │ │+ global vars │ | ||
| │+ shell path │ │+ agent config │ | ||
| │+ shell args │ │+ session vars │ | ||
| └────────┬────────┘ └────────┬────────┘ | ||
| │ │ | ||
| ▼ ▼ | ||
| ┌─────────────────┐ ┌─────────────────┐ | ||
| │ PTY Environment │ │ Child Process │ | ||
| │ Variables │ │ Environment │ | ||
| └─────────────────┘ └─────────────────┘ | ||
| ``` | ||
|
|
||
| ### Data Flow: Settings → Spawned Process | ||
|
|
||
| #### For Terminal Sessions | ||
|
|
||
| ``` | ||
| 1. User opens Settings → General → Shell Configuration | ||
| 2. Enters environment variables: KEY=VALUE (one per line) | ||
| 3. Clicks Save or Auto-Save triggers | ||
| 4. Renderer calls: window.maestro.settings.set('shellEnvVars', {...}) | ||
| 5. IPC handler persists to electron-store | ||
| 6. User spawns terminal via Maestro UI | ||
| 7. ProcessManager.spawn() called with config | ||
| 8. PtySpawner extracts shellEnvVars from config | ||
| 9. buildPtyTerminalEnv(shellEnvVars) called | ||
| 10. PTY process spawned with merged environment |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Several fences are missing a language tag, which triggers MD040.
✍️ Example fix
-```
+```text
┌─────────────────────────────────────────────────────────────┐
│ User Settings UI │
...
└─────────────────┘ └─────────────────┘- +text
- User opens Settings → General → Shell Configuration
- Enters environment variables: KEY=VALUE (one per line)
...
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 76-76: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/env-vars.md around lines 45 - 110, The markdown has fenced code blocks
without language tags (the ASCII diagram under the top diagram block and the
numbered steps under "Data Flow: Settings → Spawned Process" / "For Terminal
Sessions"); update those fences to include a language identifier (e.g., change
totext) for the diagram block and the numbered-list block so they pass
MD040 linting while preserving content and spacing.
</details>
<!-- fingerprinting:phantom:poseidon:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ### Core Implementation | ||
|
|
||
| | File | Purpose | | ||
| | ---------------------------------------------------------- | ---------------------------------- | | ||
| | `src/main/process-manager/utils/envBuilder.ts` | Environment construction functions | | ||
| | `src/main/process-manager/spawners/ChildProcessSpawner.ts` | Agent spawning with env merging | | ||
| | `src/main/process-manager/spawners/PtySpawner.ts` | Terminal spawning with env | | ||
| | `src/main/ipc/handlers/process.ts` | IPC handler loading global vars | | ||
|
|
||
| ### Settings & Storage | ||
|
|
||
| | File | Purpose | | ||
| | ------------------------------------------- | ------------------------- | | ||
| | `src/main/stores/types.ts` | MaestroSettings interface | | ||
| | `src/main/stores/defaults.ts` | Default settings values | | ||
| | `src/main/stores/instances.ts` | Store initialization | | ||
| | `src/main/preload/settings.ts` | IPC bridge for settings | | ||
| | `src/renderer/stores/settingsStore.ts` | Zustand renderer store | | ||
| | `src/renderer/components/SettingsModal.tsx` | Settings UI component | | ||
|
|
||
| ### Testing | ||
|
|
||
| | File | Purpose | | ||
| | ------------------------------------------------------------- | ----------------- | | ||
| | `src/main/process-manager/utils/__tests__/envBuilder.test.ts` | Unit tests | | ||
| | `src/__tests__/integration/process-global-env-vars.test.ts` | Integration tests | | ||
|
|
There was a problem hiding this comment.
Surround tables with blank lines.
The table blocks should be separated by blank lines to satisfy MD058.
✍️ Example fix
### Core Implementation
+
| File | Purpose |
| ---------------------------------------------------------- | ---------------------------------- |
| `src/main/process-manager/utils/envBuilder.ts` | Environment construction functions |
| `src/main/process-manager/spawners/ChildProcessSpawner.ts` | Agent spawning with env merging |
| `src/main/process-manager/spawners/PtySpawner.ts` | Terminal spawning with env |
| `src/main/ipc/handlers/process.ts` | IPC handler loading global vars |
+
### Settings & Storage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/env-vars.md` around lines 382 - 408, The markdown tables in
docs/env-vars.md are not separated by blank lines which triggers MD058; update
the file so each table block (for example the "Core Implementation", "Settings &
Storage", and "Testing" tables) is preceded and followed by a blank line,
ensuring there is an empty line before the opening | header row and after the
closing row for each table so the linter passes; no code changes required beyond
inserting those blank lines around the table blocks.
| const commitChanges = (newEntries: EnvVarEntry[]) => { | ||
| const newEnvVars: Record<string, string> = {}; | ||
| const errors: Record<number, string> = {}; | ||
| newEntries.forEach((entry) => { | ||
| const error = validateEntry(entry); | ||
| if (error) { | ||
| errors[entry.id] = error; | ||
| } | ||
| if (entry.key.trim()) { | ||
| newEnvVars[entry.key] = entry.value; | ||
| } | ||
| }); | ||
| setValidationErrors(errors); | ||
| setEnvVars(newEnvVars); |
There was a problem hiding this comment.
Validation errors should prevent persisting invalid keys.
Right now invalid keys still get written to newEnvVars, so users see an error but the bad key is saved anyway.
✅ Suggested fix
const errors: Record<number, string> = {};
newEntries.forEach((entry) => {
const error = validateEntry(entry);
if (error) {
errors[entry.id] = error;
+ return;
}
if (entry.key.trim()) {
newEnvVars[entry.key] = entry.value;
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const commitChanges = (newEntries: EnvVarEntry[]) => { | |
| const newEnvVars: Record<string, string> = {}; | |
| const errors: Record<number, string> = {}; | |
| newEntries.forEach((entry) => { | |
| const error = validateEntry(entry); | |
| if (error) { | |
| errors[entry.id] = error; | |
| } | |
| if (entry.key.trim()) { | |
| newEnvVars[entry.key] = entry.value; | |
| } | |
| }); | |
| setValidationErrors(errors); | |
| setEnvVars(newEnvVars); | |
| const commitChanges = (newEntries: EnvVarEntry[]) => { | |
| const newEnvVars: Record<string, string> = {}; | |
| const errors: Record<number, string> = {}; | |
| newEntries.forEach((entry) => { | |
| const error = validateEntry(entry); | |
| if (error) { | |
| errors[entry.id] = error; | |
| return; | |
| } | |
| if (entry.key.trim()) { | |
| newEnvVars[entry.key] = entry.value; | |
| } | |
| }); | |
| setValidationErrors(errors); | |
| setEnvVars(newEnvVars); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SettingsModal.tsx` around lines 119 - 132,
commitChanges currently writes entries into newEnvVars even when validateEntry
returns an error; update commitChanges (and use validateEntry and EnvVarEntry)
so that you only add an entry to newEnvVars when validateEntry(entry) returns
falsy (no error) and entry.key.trim() is non-empty. Keep collecting errors into
errors[entry.id] as before, call setValidationErrors(errors), and only call
setEnvVars(newEnvVars) after filtering out invalid entries.
| {/* Global Environment Variables */} | ||
| <div className="flex items-start gap-2 mb-2"> | ||
| <div className="flex-1"> | ||
| <p className="text-xs opacity-50"> | ||
| <strong>Global Environment Variables</strong> apply to all terminal | ||
| sessions and AI agent processes. Format: KEY=VALUE (one per line). | ||
| Variables with special characters should be quoted. Agent-specific | ||
| settings can override these values. Typical use cases: API keys, proxy | ||
| settings, custom tool paths. | ||
| </p> | ||
| </div> | ||
| <div | ||
| className="group relative flex-shrink-0 mt-0.5" | ||
| title="Environment variables configured here are available to all terminal sessions, all AI agent processes (Claude, OpenCode, etc.), and any spawned child processes. Agent-specific settings can override these values." | ||
| > | ||
| <HelpCircle | ||
| className="w-4 h-4 cursor-help" | ||
| style={{ color: theme.colors.textDim }} | ||
| /> | ||
| <div className="absolute hidden group-hover:block bg-black/80 text-white text-xs rounded p-2 z-50 w-60 -right-2 top-5 whitespace-normal"> | ||
| <p className="mb-1 font-semibold">Environment variables apply to:</p> | ||
| <ul className="list-disc list-inside space-y-0.5"> | ||
| <li>All terminal sessions</li> | ||
| <li>All AI agent processes (Claude, OpenCode, etc.)</li> | ||
| <li>Any spawned child processes</li> | ||
| </ul> | ||
| <p className="mt-1">Agent-specific settings can override these values.</p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Make the help tooltip keyboard-accessible.
It only appears on hover; add focus support and a focusable target.
♿ Suggested fix
- <div
- className="group relative flex-shrink-0 mt-0.5"
+ <div
+ className="group relative flex-shrink-0 mt-0.5 outline-none"
+ tabIndex={0}
title="Environment variables configured here are available to all terminal sessions, all AI agent processes (Claude, OpenCode, etc.), and any spawned child processes. Agent-specific settings can override these values."
>
<HelpCircle
className="w-4 h-4 cursor-help"
style={{ color: theme.colors.textDim }}
/>
- <div className="absolute hidden group-hover:block bg-black/80 text-white text-xs rounded p-2 z-50 w-60 -right-2 top-5 whitespace-normal">
+ <div className="absolute hidden group-hover:block group-focus-within:block bg-black/80 text-white text-xs rounded p-2 z-50 w-60 -right-2 top-5 whitespace-normal">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* Global Environment Variables */} | |
| <div className="flex items-start gap-2 mb-2"> | |
| <div className="flex-1"> | |
| <p className="text-xs opacity-50"> | |
| <strong>Global Environment Variables</strong> apply to all terminal | |
| sessions and AI agent processes. Format: KEY=VALUE (one per line). | |
| Variables with special characters should be quoted. Agent-specific | |
| settings can override these values. Typical use cases: API keys, proxy | |
| settings, custom tool paths. | |
| </p> | |
| </div> | |
| <div | |
| className="group relative flex-shrink-0 mt-0.5" | |
| title="Environment variables configured here are available to all terminal sessions, all AI agent processes (Claude, OpenCode, etc.), and any spawned child processes. Agent-specific settings can override these values." | |
| > | |
| <HelpCircle | |
| className="w-4 h-4 cursor-help" | |
| style={{ color: theme.colors.textDim }} | |
| /> | |
| <div className="absolute hidden group-hover:block bg-black/80 text-white text-xs rounded p-2 z-50 w-60 -right-2 top-5 whitespace-normal"> | |
| <p className="mb-1 font-semibold">Environment variables apply to:</p> | |
| <ul className="list-disc list-inside space-y-0.5"> | |
| <li>All terminal sessions</li> | |
| <li>All AI agent processes (Claude, OpenCode, etc.)</li> | |
| <li>Any spawned child processes</li> | |
| </ul> | |
| <p className="mt-1">Agent-specific settings can override these values.</p> | |
| </div> | |
| </div> | |
| {/* Global Environment Variables */} | |
| <div className="flex items-start gap-2 mb-2"> | |
| <div className="flex-1"> | |
| <p className="text-xs opacity-50"> | |
| <strong>Global Environment Variables</strong> apply to all terminal | |
| sessions and AI agent processes. Format: KEY=VALUE (one per line). | |
| Variables with special characters should be quoted. Agent-specific | |
| settings can override these values. Typical use cases: API keys, proxy | |
| settings, custom tool paths. | |
| </p> | |
| </div> | |
| <div | |
| className="group relative flex-shrink-0 mt-0.5 outline-none" | |
| tabIndex={0} | |
| title="Environment variables configured here are available to all terminal sessions, all AI agent processes (Claude, OpenCode, etc.), and any spawned child processes. Agent-specific settings can override these values." | |
| > | |
| <HelpCircle | |
| className="w-4 h-4 cursor-help" | |
| style={{ color: theme.colors.textDim }} | |
| /> | |
| <div className="absolute hidden group-hover:block group-focus-within:block bg-black/80 text-white text-xs rounded p-2 z-50 w-60 -right-2 top-5 whitespace-normal"> | |
| <p className="mb-1 font-semibold">Environment variables apply to:</p> | |
| <ul className="list-disc list-inside space-y-0.5"> | |
| <li>All terminal sessions</li> | |
| <li>All AI agent processes (Claude, OpenCode, etc.)</li> | |
| <li>Any spawned child processes</li> | |
| </ul> | |
| <p className="mt-1">Agent-specific settings can override these values.</p> | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/SettingsModal.tsx` around lines 1477 - 1505, The
tooltip trigger (the wrapper div with className "group relative ..." containing
HelpCircle) is only hover-activated and not keyboard-accessible; make it
focusable by adding tabIndex={0} (or tabIndex={-1} if you prefer programmatic
focus) and ensure keyboard focus displays the tooltip by relying on the existing
"group-hover:block" behavior—replace or augment with a focus-visible/focus:block
approach (e.g., use group-focus:block or add a focused class) so the sibling
tooltip div becomes visible on focus as well; also add outline-none to the
trigger if following the style guideline and ensure the tooltip content remains
reachable/readable for screen readers.
- SSH handler: Merge globalShellEnvVars with effectiveCustomEnvVars before passing to buildSshCommandWithStdin (session vars take precedence) - PTY spawner: Merge shellEnvVars with process.env for non-terminal agents to ensure global env vars are applied - Both fixes maintain proper precedence: session vars > global vars > process env
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/process-manager/spawners/PtySpawner.ts`:
- Around line 88-90: The PTY spawner currently only merges shellEnvVars into
ptyEnv and omits session-level customEnvVars; extract customEnvVars (and
isResuming if available) from the config in PtySpawner.ts and include them when
building ptyEnv—either call the existing buildChildProcessEnv(customEnvVars,
isResuming, shellEnvVars) like ChildProcessSpawner does, or merge with correct
precedence (ptyEnv = { ...process.env, ...shellEnvVars, ...customEnvVars }) so
customEnvVars override shell/global vars.
| // For AI agents in PTY mode: merge global shell env vars with process env | ||
| // This ensures agents receive both system env vars (NODE_PATH, etc.) and global env vars | ||
| ptyEnv = shellEnvVars ? { ...process.env, ...shellEnvVars } : process.env; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how customEnvVars flows through ProcessManager and spawners
echo "=== Check ProcessConfig type for customEnvVars ==="
rg -n "customEnvVars" --type=ts -C3 src/main/process-manager/
echo ""
echo "=== Check where PtySpawner is called and what config is passed ==="
rg -n "PtySpawner" --type=ts src/main/process-manager/ -C2
echo ""
echo "=== Check ProcessConfig interface ==="
rg -n "interface ProcessConfig\|type ProcessConfig" --type=ts -A20 src/main/Repository: RunMaestro/Maestro
Length of output: 6970
🏁 Script executed:
#!/bin/bash
# Check PtySpawner around lines 88-90
echo "=== PtySpawner.ts around lines 88-90 ==="
sed -n '80,100p' src/main/process-manager/spawners/PtySpawner.ts | cat -n
echo ""
echo "=== Check if buildPtyTerminalEnv exists ==="
rg -n "buildPtyTerminalEnv" --type=ts src/main/process-manager/
echo ""
echo "=== Check PtySpawner.spawn signature and customEnvVars usage ==="
rg -n "spawn\|customEnvVars" --type=ts -A3 -B3 src/main/process-manager/spawners/PtySpawner.ts | head -80
echo ""
echo "=== Full PtySpawner constructor and spawn method signature ==="
sed -n '1,150p' src/main/process-manager/spawners/PtySpawner.ts | cat -nRepository: RunMaestro/Maestro
Length of output: 7959
🏁 Script executed:
#!/bin/bash
# Check buildPtyTerminalEnv function
echo "=== buildPtyTerminalEnv function implementation ==="
sed -n '63,100p' src/main/process-manager/utils/envBuilder.ts | cat -n
echo ""
echo "=== Full buildPtyTerminalEnv including full function ==="
sed -n '40,130p' src/main/process-manager/utils/envBuilder.ts | cat -nRepository: RunMaestro/Maestro
Length of output: 5406
Add customEnvVars support to PTY agents—currently missing session-level custom env vars.
For AI agents requiring PTY, the current merge at lines 88-90 only includes shellEnvVars (global), not customEnvVars (session-level). This differs from ChildProcessSpawner, which properly merges both via buildChildProcessEnv(customEnvVars, isResuming, shellEnvVars).
Extract customEnvVars from config and include it in the merge with appropriate precedence:
Fix
- const { sessionId, toolType, cwd, command, args, shell, shellArgs, shellEnvVars } = config;
+ const { sessionId, toolType, cwd, command, args, shell, shellArgs, shellEnvVars, customEnvVars } = config;
[... lines 63-87 ...]
- // For AI agents in PTY mode: merge global shell env vars with process env
- // This ensures agents receive both system env vars (NODE_PATH, etc.) and global env vars
- ptyEnv = shellEnvVars ? { ...process.env, ...shellEnvVars } : process.env;
+ // For AI agents in PTY mode: merge process env with global and session-level env vars
+ // Precedence: session vars > global vars > process.env
+ ptyEnv = { ...process.env, ...shellEnvVars, ...customEnvVars };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/process-manager/spawners/PtySpawner.ts` around lines 88 - 90, The
PTY spawner currently only merges shellEnvVars into ptyEnv and omits
session-level customEnvVars; extract customEnvVars (and isResuming if available)
from the config in PtySpawner.ts and include them when building ptyEnv—either
call the existing buildChildProcessEnv(customEnvVars, isResuming, shellEnvVars)
like ChildProcessSpawner does, or merge with correct precedence (ptyEnv = {
...process.env, ...shellEnvVars, ...customEnvVars }) so customEnvVars override
shell/global vars.
This pull request introduces a major enhancement to Maestro's environment variable management: global environment variables configured in Settings now apply to all AI agent processes, not just terminal sessions. This enables centralized configuration of API keys, proxies, tool paths, and more, streamlining automation and agent workflows. The documentation and UI have been updated to clarify this behavior and provide detailed usage instructions, and the codebase has been modified to ensure these variables are correctly applied and logged.
Global Environment Variable Support:
Documentation and UI Updates:
docs/configuration.mdanddocs/features.mdexplaining how to configure and use global environment variables, including precedence, example use cases, and override mechanisms. [1] [2] [3]Codebase Changes:
Other Documentation Improvements:
docs/configuration.mdfor consistency and readability, including notification, sync, and platform support sections. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]These changes provide a more powerful and user-friendly way to manage environment variables across all Maestro workflows, improving automation, security, and portability.
Issues
closes #372
Summary by CodeRabbit
New Features
Documentation